Drop duplicate instance tags, if exists#78
Drop duplicate instance tags, if exists#78openshift-merge-robot merged 1 commit intoopenshift:masterfrom trawler:remove_duplicated_tags
Conversation
There was a problem hiding this comment.
This, I think, can be reduced to:
...
keys := make(map[string]bool)
result := []*ec2.Tag{}
for _, entry := range tags {
if _, value := keys[*entry.Key]; !value {
keys[*entry.Key] = true
result = append(result, entry)
}
}
return result
}
but untested - I just typed it out!
Please could you also create a unit test for this function as it it generic.
There was a problem hiding this comment.
This, I think, can be reduced to:
...
keys := make(map[string]bool)
result := []*ec2.Tag{}
for _, entry := range tags {
if _, value := keys[*entry.Key]; !value {
keys[*entry.Key] = true
result = append(result, entry)
}
}
return result
}
but untested - I just typed it out!
Please could you also create a unit test for this function as it it generic.
enxebre
left a comment
There was a problem hiding this comment.
looking great! can we please include a unit test for this?
|
/test e2e-aws |
There was a problem hiding this comment.
The length doesn't guarantee correctness. Better to have expected and actual and use reflect.DeepEquals to verify the result.
There was a problem hiding this comment.
Add the empty case (i.e., empty slices).
There was a problem hiding this comment.
I think it would be more succinct to reduce all of this output to just:
t.Errorf("test #%d: expected %+v, got %+v", c.expected, actual)
as the inputs are quite small.
| rawTagList = append(rawTagList, &ec2.Tag{Key: aws.String(tag.Name), Value: aws.String(tag.Value)}) | ||
| } | ||
| tagList = append(tagList, []*ec2.Tag{ | ||
| rawTagList = append(rawTagList, []*ec2.Tag{ |
There was a problem hiding this comment.
Can't we just put all the tags under a map and then check if the key already exists?
tagMap := make(map[string]struct{})
for _, tag := range machineProviderConfig.Tags {
tagMap[tag.Name] = struct{}{}
}
if _, exists := tagMap["clusterid"]; !exists {
tagList = append(tagList, {Key: aws.String("clusterid"), Value: aws.String(clusterID)}}
}
...There was a problem hiding this comment.
personally I like current implementation. It prevents also from injecting duplications @frobware wdyt?
There was a problem hiding this comment.
I kind of prefer the de-dupe phase. It's easier to rationalise. I have a bunch of tags, then a last action is to de-dupe them.
|
/lgtm |
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: frobware The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
related to: openshift/installer#468 (comment)